Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use gix pipeline filter instead of manual crlf implementation #9503

Merged
merged 2 commits into from
Feb 3, 2024

Conversation

ShoyuVanilla
Copy link
Contributor

Fixes #8145

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-vcs Area: Version control system interaction labels Feb 2, 2024
// Get the actual data that git would make out of the git object.
// This will apply the user's git config or attributes like crlf conversions.
if let Some(work_dir) = repo.work_dir() {
let rela_path = file.strip_prefix(work_dir)?.to_string_lossy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing lossy conversion is incorrect. Gix provides some utilities for proper path -> byte conversion.

let rela_path = file.strip_prefix(work_dir)?.to_string_lossy();
let (mut pipeline, _) = repo.filter_pipeline(None)?;
let mut worktree_outcome =
pipeline.convert_to_worktree(&data, rela_path.as_ref().into(), Delay::Forbid)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Byron how does this handle encoding? Git stores everything as utf-8 internally IIRC and only converts back on checkout.

I guess that this function would do that? Could we instead checkout utf-8 somehow. (My memory is very cloudy here so I may be missremebering)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the case, but it's true that Git has an 'internal' form that it wants to store in the object database. This is merely a form that passed through the filter pipeline though, which on Unix is most often a no-op.

It's true though that this method takes care of transforming data which is supposed to be directly from the object database and turn it into what would be checked out, applying all necessary transformations.

@@ -448,6 +448,18 @@ dependencies = [
"winapi",
]

[[package]]
name = "filetime"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of new dependencies :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It comes from new feature flag gix = { features = ["attributes"], .. }, which is needed for https://docs.rs/gix/0.58.0/gix/struct.Repository.html#method.filter_pipeline 😢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed how this dependency is used and it seems 'convenience' is the answer. No feature is used that wouldn't be covered by the doing what the crate does.

I added a task to the helix tracking issue to represent that possibility, and help would definitely be welcome.

Copy link
Contributor

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took my time to study the PR and believe that't the way to do it! Good work :)!

Additionally I was thinking that using the worktree-version as base for a diff is also exactly would should be done given how the user opens and edits files in the worktree.

For posterity, let me mention that by now there is also the repo.diff_recource_cache() method which allows to perform diffs just like diff does which, you guessed it, may apply even more transformations 😁. Doing so would be wrong here, but if there ever is a 'show me the git diff' function in helix, that would be the way to do it (and that's particularly interesting if the user configured diff-filters to turn binary files into something diffable).

if let Some(work_dir) = repo.work_dir() {
let rela_path = file.strip_prefix(work_dir)?;
let rela_path = gix::path::try_into_bstr(rela_path)?;
let (mut pipeline, _) = repo.filter_pipeline(None)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a note: It's a bit sad that the filter pipeline can't be reused under the circumstances. if this should ever be an issue that can be achieved by dropping down to the internal plumbing types.

However, I wouldn't go there as this will seriously complicate the code - I just wanted you to know that you can to shave off some cycles.

let rela_path = file.strip_prefix(work_dir)?.to_string_lossy();
let (mut pipeline, _) = repo.filter_pipeline(None)?;
let mut worktree_outcome =
pipeline.convert_to_worktree(&data, rela_path.as_ref().into(), Delay::Forbid)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the case, but it's true that Git has an 'internal' form that it wants to store in the object database. This is merely a form that passed through the filter pipeline though, which on Unix is most often a no-op.

It's true though that this method takes care of transforming data which is supposed to be directly from the object database and turn it into what would be checked out, applying all necessary transformations.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This fixes the diff gutter for me in a repo where I use git-crypt 🚀

@the-mikedavis the-mikedavis merged commit 81ae768 into helix-editor:master Feb 3, 2024
6 checks passed
@ShoyuVanilla ShoyuVanilla deleted the gix-filter-pipeline branch February 4, 2024 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-vcs Area: Version control system interaction S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
5 participants